[rustc_span][perf] Hoist lookup sorted by words out of the loop. #114395
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
@lqd commented on rust-lang#114351 asking if `sort_by_words(lookup)` is computed repeatedly. I was assuming that rustc should have no difficulties to hoist it automatically outside of the loop to avoid repeated pure computation, but according to https://godbolt.org/z/frs8Kj1rq it seems like I was wrong: original version seems to have 2 calls per loop iteration ``` .LBB16_3: mov rbx, qword ptr [r13] mov r14, qword ptr [r13 + 8] lea rdi, [rsp + 40] mov rsi, rbx mov rdx, r14 call example::sort_by_words lea rdi, [rsp + 64] mov rsi, qword ptr [rsp + 8] mov rdx, qword ptr [rsp + 16] call example::sort_by_words mov rdi, qword ptr [rsp + 40] mov rdx, qword ptr [rsp + 56] mov rsi, qword ptr [rsp + 64] cmp rdx, qword ptr [rsp + 80] mov qword ptr [rsp + 32], rdi mov qword ptr [rsp + 24], rsi jne .LBB16_5 call qword ptr [rip + bcmp@GOTPCREL] test eax, eax sete al mov dword ptr [rsp + 4], eax mov rsi, qword ptr [rsp + 72] test rsi, rsi jne .LBB16_8 jmp .LBB16_9 ``` but the manually hoisted version just 1: ``` .LBB16_3: mov r13, qword ptr [r15] mov r14, qword ptr [r15 + 8] lea rdi, [rsp + 64] mov rsi, r13 mov rdx, r14 call example::sort_by_words mov rdi, qword ptr [rsp + 64] mov rdx, qword ptr [rsp + 16] cmp qword ptr [rsp + 80], rdx mov qword ptr [rsp + 32], rdi jne .LBB16_5 mov rsi, qword ptr [rsp + 8] call qword ptr [rip + bcmp@GOTPCREL] test eax, eax sete bpl mov rsi, qword ptr [rsp + 72] test rsi, rsi jne .LBB16_8 jmp .LBB16_9 ``` This code is probably not very hot, but there is no reason to leave such a low hanging fruit.
|
r? @eholk (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Great, thank you! As we said on the other PR, this code is quite cold, and IIUC only used in diagnostics: there shouldn't be any perf impact, so let's roll it up. r? lqd @bors r+ rollup |
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Aug 3, 2023
[rustc_span][perf] Hoist lookup sorted by words out of the loop. `@lqd` commented on rust-lang#114351 asking if `sort_by_words(lookup)` is computed repeatedly. I was assuming that rustc should have no difficulties to hoist it automatically outside of the loop to avoid repeated pure computation, but according to https://godbolt.org/z/frs8Kj1rq it seems like I was wrong: original version seems to have 2 calls per loop iteration ``` .LBB16_3: mov rbx, qword ptr [r13] mov r14, qword ptr [r13 + 8] lea rdi, [rsp + 40] mov rsi, rbx mov rdx, r14 call example::sort_by_words lea rdi, [rsp + 64] mov rsi, qword ptr [rsp + 8] mov rdx, qword ptr [rsp + 16] call example::sort_by_words mov rdi, qword ptr [rsp + 40] mov rdx, qword ptr [rsp + 56] mov rsi, qword ptr [rsp + 64] cmp rdx, qword ptr [rsp + 80] mov qword ptr [rsp + 32], rdi mov qword ptr [rsp + 24], rsi jne .LBB16_5 call qword ptr [rip + bcmp@GOTPCREL] test eax, eax sete al mov dword ptr [rsp + 4], eax mov rsi, qword ptr [rsp + 72] test rsi, rsi jne .LBB16_8 jmp .LBB16_9 ``` but the manually hoisted version just 1: ``` .LBB16_3: mov r13, qword ptr [r15] mov r14, qword ptr [r15 + 8] lea rdi, [rsp + 64] mov rsi, r13 mov rdx, r14 call example::sort_by_words mov rdi, qword ptr [rsp + 64] mov rdx, qword ptr [rsp + 16] cmp qword ptr [rsp + 80], rdx mov qword ptr [rsp + 32], rdi jne .LBB16_5 mov rsi, qword ptr [rsp + 8] call qword ptr [rip + bcmp@GOTPCREL] test eax, eax sete bpl mov rsi, qword ptr [rsp + 72] test rsi, rsi jne .LBB16_8 jmp .LBB16_9 ``` This code is probably not very hot, but there is no reason to leave such a low hanging fruit.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Aug 3, 2023
[rustc_span][perf] Hoist lookup sorted by words out of the loop. ``@lqd`` commented on rust-lang#114351 asking if `sort_by_words(lookup)` is computed repeatedly. I was assuming that rustc should have no difficulties to hoist it automatically outside of the loop to avoid repeated pure computation, but according to https://godbolt.org/z/frs8Kj1rq it seems like I was wrong: original version seems to have 2 calls per loop iteration ``` .LBB16_3: mov rbx, qword ptr [r13] mov r14, qword ptr [r13 + 8] lea rdi, [rsp + 40] mov rsi, rbx mov rdx, r14 call example::sort_by_words lea rdi, [rsp + 64] mov rsi, qword ptr [rsp + 8] mov rdx, qword ptr [rsp + 16] call example::sort_by_words mov rdi, qword ptr [rsp + 40] mov rdx, qword ptr [rsp + 56] mov rsi, qword ptr [rsp + 64] cmp rdx, qword ptr [rsp + 80] mov qword ptr [rsp + 32], rdi mov qword ptr [rsp + 24], rsi jne .LBB16_5 call qword ptr [rip + bcmp@GOTPCREL] test eax, eax sete al mov dword ptr [rsp + 4], eax mov rsi, qword ptr [rsp + 72] test rsi, rsi jne .LBB16_8 jmp .LBB16_9 ``` but the manually hoisted version just 1: ``` .LBB16_3: mov r13, qword ptr [r15] mov r14, qword ptr [r15 + 8] lea rdi, [rsp + 64] mov rsi, r13 mov rdx, r14 call example::sort_by_words mov rdi, qword ptr [rsp + 64] mov rdx, qword ptr [rsp + 16] cmp qword ptr [rsp + 80], rdx mov qword ptr [rsp + 32], rdi jne .LBB16_5 mov rsi, qword ptr [rsp + 8] call qword ptr [rip + bcmp@GOTPCREL] test eax, eax sete bpl mov rsi, qword ptr [rsp + 72] test rsi, rsi jne .LBB16_8 jmp .LBB16_9 ``` This code is probably not very hot, but there is no reason to leave such a low hanging fruit.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 3, 2023
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#113657 (Expand, rename and improve `incorrect_fn_null_checks` lint) - rust-lang#114237 (parser: more friendly hints for handling `async move` in the 2015 edition) - rust-lang#114300 (Suggests turbofish in patterns) - rust-lang#114372 (const validation: point at where we found a pointer but expected an integer) - rust-lang#114395 ([rustc_span][perf] Hoist lookup sorted by words out of the loop.) - rust-lang#114403 (fix the span in the suggestion of remove question mark) - rust-lang#114408 (Temporary remove myself from review rotation) - rust-lang#114415 (Skip checking of `rustc_codegen_gcc` with vendoring enabled) r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@lqd commented on #114351 asking if
sort_by_words(lookup)is computed repeatedly. I was assuming that rustc should have no difficulties to hoist it automatically outside of the loop to avoid repeated pure computation, but according tohttps://godbolt.org/z/frs8Kj1rq it seems like I was wrong:
original version seems to have 2 calls per loop iteration
but the manually hoisted version just 1:
This code is probably not very hot, but there is no reason to leave such a low hanging fruit.